refactor(turbopack): Add `NonLocalValue` derives to types deriving TraceRawVcs #73714

bgw · 2024-12-10T01:44:37Z

Types that implement TraceRawVcs do so because they aren't a VcValueType, but they are used inside of a VcValueType.

We have a similar issue for the NonLocalValue (formerly ResolvedValue) marker trait. We need it to be implemented for all things included inside of VcValueTypes, so that we can know that VcValueTypes do not recursively contain potentially-local Vcs.

I started this PR by adding NonLocalValue to all things that explicitly derive TraceRawVcs:

sg run --lang rust --pattern '#[derive($$$BEFORE TraceRawVcs $$$AFTER)]' --rewrite '#[derive($$$BEFORE TraceRawVcs $$$AFTER, NonLocalValue)]' --update-all
fastmod ',, NonLocalValue' ', NonLocalValue'       # fix up some double comments caused by the astgrep transform above

And then I went through doing the following:

Adding imports of NonLocalValue.
Removing the NonLocalValue in cases where it's not easy for the struct to implement it yet.
Adding non_local options to #[turbo_task::value] macros in places where it would help us derive NonLocalValue somewhere else.
In a few cases removed some #[turbo_tasks(trace_ignore)]s in favor of adding a proper TraceRawVcs and NonLocalValue implementations.

Included with this are a few improvements to the turbo-tasks and turbo-tasks-macros crates:

Fixed an issue where rust-analyzer (but not cargo check) would report a warning about non_snake_case inside the derive macro.
Remove some type bounds from the AdjacencyMap struct (see refactor(turbo-tasks): Simplify most type bounds on Vc<T> and related types #72823 for an explaination) and implement NonLocalValue on it.
Implement NonLocalValue (and TraceRawVcs) on a few more primitive types.

Closes PACK-3639

bgw · 2024-12-10T01:44:50Z

refactor(turbopack): Add NonLocalValue derives to types deriving TraceRawVcs #73714 👈 (View in Graphite)
docs(turbopack): Better document the Vc type, with references to ResolvedVc and VcOperation #72524
refactor(turbo-tasks): Rename ResolvedValue trait to NonLocalValue #73191
refactor(turbo-tasks): Introduce a OperationValue trait, generalized from the ResolvedValue trait #72788 : 1 other dependent PR (#72871 )
[Turbopack] Introduce OperationVc that wraps operations #70242 : 1 other dependent PR (#72776 )
canary

This stack of pull requests is managed by Graphite. Learn more about stacking.

bgw · 2024-12-10T18:58:42Z

Merge activity

Dec 10, 1:58 PM EST: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..
Dec 10, 4:47 PM EST: A user merged this pull request with Graphite.

ijjk · 2024-12-10T19:28:45Z

Tests Passed

ijjk · 2024-12-10T19:30:35Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js bgw/common-types-non-local-value	Change
buildDuration	30s	26.9s	N/A
buildDurationCached	22.8s	21.5s	N/A
nodeModulesSize	409 MB	409 MB	✓
nextStartRea..uration (ms)	723ms	872ms	⚠️ +149ms

Client Bundles (main, webpack)

	vercel/next.js canary	vercel/next.js bgw/common-types-non-local-value	Change
1187-HASH.js gzip	50.4 kB	50.4 kB	N/A
8276.HASH.js gzip	169 B	168 B	N/A
8377-HASH.js gzip	5.3 kB	5.3 kB	N/A
bccd1874-HASH.js gzip	53 kB	53 kB	N/A
framework-HASH.js gzip	57.5 kB	57.5 kB	N/A
main-app-HASH.js gzip	232 B	235 B	N/A
main-HASH.js gzip	33.8 kB	33.7 kB	N/A
webpack-HASH.js gzip	1.71 kB	1.71 kB	N/A
Overall change	0 B	0 B	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	vercel/next.js bgw/common-types-non-local-value	Change
polyfills-HASH.js gzip	39.4 kB	39.4 kB	✓
Overall change	39.4 kB	39.4 kB	✓

Client Pages

	vercel/next.js canary	vercel/next.js bgw/common-types-non-local-value	Change
_app-HASH.js gzip	193 B	193 B	✓
_error-HASH.js gzip	193 B	193 B	✓
amp-HASH.js gzip	512 B	510 B	N/A
css-HASH.js gzip	343 B	342 B	N/A
dynamic-HASH.js gzip	1.84 kB	1.84 kB	✓
edge-ssr-HASH.js gzip	265 B	265 B	✓
head-HASH.js gzip	363 B	362 B	N/A
hooks-HASH.js gzip	393 B	392 B	N/A
image-HASH.js gzip	4.44 kB	4.43 kB	N/A
index-HASH.js gzip	268 B	268 B	✓
link-HASH.js gzip	2.35 kB	2.34 kB	N/A
routerDirect..HASH.js gzip	328 B	328 B	✓
script-HASH.js gzip	397 B	397 B	✓
withRouter-HASH.js gzip	323 B	326 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	3.59 kB	3.59 kB	✓

Client Build Manifests

	vercel/next.js canary	vercel/next.js bgw/common-types-non-local-value	Change
_buildManifest.js gzip	747 B	745 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js bgw/common-types-non-local-value	Change
index.html gzip	524 B	523 B	N/A
link.html gzip	539 B	537 B	N/A
withRouter.html gzip	520 B	520 B	✓
Overall change	520 B	520 B	✓

Edge SSR bundle Size

	vercel/next.js canary	vercel/next.js bgw/common-types-non-local-value	Change
edge-ssr.js gzip	128 kB	128 kB	N/A
page.js gzip	203 kB	203 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	vercel/next.js bgw/common-types-non-local-value	Change
middleware-b..fest.js gzip	669 B	667 B	N/A
middleware-r..fest.js gzip	155 B	156 B	N/A
middleware.js gzip	31 kB	31 kB	N/A
edge-runtime..pack.js gzip	844 B	844 B	✓
Overall change	844 B	844 B	✓

Next Runtimes

	vercel/next.js canary	vercel/next.js bgw/common-types-non-local-value	Change
523-experime...dev.js gzip	322 B	322 B	✓
523.runtime.dev.js gzip	314 B	314 B	✓
app-page-exp...dev.js gzip	322 kB	322 kB	✓
app-page-exp..prod.js gzip	127 kB	127 kB	✓
app-page-tur..prod.js gzip	140 kB	140 kB	✓
app-page-tur..prod.js gzip	135 kB	135 kB	✓
app-page.run...dev.js gzip	312 kB	312 kB	✓
app-page.run..prod.js gzip	122 kB	122 kB	✓
app-route-ex...dev.js gzip	37.1 kB	37.1 kB	✓
app-route-ex..prod.js gzip	25.1 kB	25.1 kB	✓
app-route-tu..prod.js gzip	25.1 kB	25.1 kB	✓
app-route-tu..prod.js gzip	24.9 kB	24.9 kB	✓
app-route.ru...dev.js gzip	38.7 kB	38.7 kB	✓
app-route.ru..prod.js gzip	24.9 kB	24.9 kB	✓
pages-api-tu..prod.js gzip	9.56 kB	9.56 kB	✓
pages-api.ru...dev.js gzip	11.4 kB	11.4 kB	✓
pages-api.ru..prod.js gzip	9.56 kB	9.56 kB	✓
pages-turbo...prod.js gzip	21.3 kB	21.3 kB	✓
pages.runtim...dev.js gzip	27 kB	27 kB	✓
pages.runtim..prod.js gzip	21.3 kB	21.3 kB	✓
server.runti..prod.js gzip	916 kB	916 kB	✓
Overall change	2.35 MB	2.35 MB	✓

build cache Overall increase ⚠️

	vercel/next.js canary	vercel/next.js bgw/common-types-non-local-value	Change
0.pack gzip	2.03 MB	2.04 MB	⚠️ +2.53 kB
index.pack gzip	73 kB	72.2 kB	N/A
Overall change	2.03 MB	2.04 MB	⚠️ +2.53 kB

Diff details

Diff for main-HASH.js

Diff too large to display

Commit: 514401c

…aceRawVcs

…c`s and `OperationVc`s (#73764) This is a follow-up to #73714 **What is NonLocalValue?** https://turbopack-rust-docs.vercel.sh/rustdoc/turbo_tasks/trait.NonLocalValue.html The core change here is this bit in `turbopack/crates/turbo-tasks/src/vc/local.rs`: ```diff - unsafe impl<T: ?Sized + NonLocalValue> NonLocalValue for OperationVc<T> {} - unsafe impl<T: ?Sized + NonLocalValue> NonLocalValue for ResolvedVc<T> {} + unsafe impl<T: ?Sized> NonLocalValue for OperationVc<T> {} + unsafe impl<T: ?Sized> NonLocalValue for ResolvedVc<T> {} ``` These new implementations are intentionally incorrect, as these values `T` could contain references to local `Vc` values. We must also check that `T: NonLocalValue`. However, we're temporarily ignoring that problem, as: - We don't *currently* depend on `NonLocalValue` for safety (local tasks aren't enabled). - We intend to make all `VcValueType`s implement `NonLocalValue`, so implementing this for all values is approximating that future state. - Adding a `T: NonLocalValue` bound introduces a lot of noise that isn't directly actionable for types that include a `ResolvedVc` or `OperationVc` that is not *yet* a `NonLocalValue`. This PR also adds the `NonLocalValue` to types explicitly deriving `TraceRawVcs`, for types that couldn't before, by using the new weaker bounds on the `NonLocalValue` impls.

ijjk added created-by: Turbopack team PRs by the Turbopack team. Font (next/font) Related to Next.js Font Optimization. Turbopack Related to Turbopack with Next.js. labels Dec 10, 2024

bgw mentioned this pull request Dec 10, 2024

docs(turbopack): Better document the Vc type, with references to ResolvedVc and VcOperation #72524

Merged

bgw mentioned this pull request Dec 10, 2024

refactor(turbo-tasks): Rename ResolvedValue trait to NonLocalValue #73191

Merged

bgw requested review from kdy1 and mischnic December 10, 2024 01:47

bgw marked this pull request as ready for review December 10, 2024 01:47

kdy1 approved these changes Dec 10, 2024

View reviewed changes

bgw changed the base branch from bgw/docs-vc-type to graphite-base/73714 December 10, 2024 18:57

bgw force-pushed the bgw/common-types-non-local-value branch from d25fa43 to dc6e704 Compare December 10, 2024 19:16

bgw force-pushed the graphite-base/73714 branch from 3498e7d to 4074ede Compare December 10, 2024 19:16

bgw changed the base branch from graphite-base/73714 to canary December 10, 2024 19:16

refactor(turbopack): Add NonLocalValue derives to types deriving Tr…

514401c

…aceRawVcs

bgw force-pushed the bgw/common-types-non-local-value branch from dc6e704 to 514401c Compare December 10, 2024 20:55

bgw merged commit e098755 into canary Dec 10, 2024
103 of 112 checks passed

bgw deleted the bgw/common-types-non-local-value branch December 10, 2024 21:47

bgw mentioned this pull request Dec 10, 2024

refactor(turbo-tasks): Implement NonLocalValue for *all* ResolvedVcs and OperationVcs #73764

Merged

github-actions bot added the locked label Dec 25, 2024

github-actions bot locked as resolved and limited conversation to collaborators Dec 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(turbopack): Add `NonLocalValue` derives to types deriving TraceRawVcs #73714

refactor(turbopack): Add `NonLocalValue` derives to types deriving TraceRawVcs #73714

bgw commented Dec 10, 2024 •

edited by padmaia

Loading

bgw commented Dec 10, 2024 •

edited

Loading

bgw commented Dec 10, 2024 •

edited

Loading

ijjk commented Dec 10, 2024 •

edited

Loading

ijjk commented Dec 10, 2024 •

edited

Loading

refactor(turbopack): Add NonLocalValue derives to types deriving TraceRawVcs #73714

refactor(turbopack): Add NonLocalValue derives to types deriving TraceRawVcs #73714

Conversation

bgw commented Dec 10, 2024 • edited by padmaia Loading

bgw commented Dec 10, 2024 • edited Loading

bgw commented Dec 10, 2024 • edited Loading

Merge activity

ijjk commented Dec 10, 2024 • edited Loading

Tests Passed

ijjk commented Dec 10, 2024 • edited Loading

Stats from current PR

refactor(turbopack): Add `NonLocalValue` derives to types deriving TraceRawVcs #73714

refactor(turbopack): Add `NonLocalValue` derives to types deriving TraceRawVcs #73714

bgw commented Dec 10, 2024 •

edited by padmaia

Loading

bgw commented Dec 10, 2024 •

edited

Loading

bgw commented Dec 10, 2024 •

edited

Loading

ijjk commented Dec 10, 2024 •

edited

Loading

ijjk commented Dec 10, 2024 •

edited

Loading